Add Exact Match option to Suggest #15410
Conversation
clintongormley
added
>enhancement
review
:Search/Suggesters
|
@areek could you have a look at this please. |
| @@ -56,7 +59,10 @@ public TermSuggestion innerExecute(String name, TermSuggestionContext suggestion | |||
| ); | |||
| Text key = new BytesText(new BytesArray(token.term.bytes())); | |||
| TermSuggestion.Entry resultEntry = new TermSuggestion.Entry(key, token.startOffset, token.endOffset - token.startOffset); | |||
| for (SuggestWord suggestWord : suggestedWords) { | |||
| if(suggestion.getDirectSpellCheckerSettings().exactMatch()){ | |||
| addExactMatch( suggestion, indexReader, token, resultEntry); | |||
There was a problem hiding this comment.
I think we can remove addExactMatch method altogether and just use the TermsEnum#seekExact to see if the term is present in the term dictionary:
final TermsEnum termsEnum = MultiFields.getTerms(indexReader, token.term.field()).iterator();
if (termsEnum.seekExact(token.term.bytes())) {
Text word = new StringText(token.term.text());
resultEntry.addOption(new TermSuggestion.Entry.Option(word, termsEnum.docFreq(), 1f));
}
|
Thanks @ricardocerq for the PR :). I left some comments. Would be awesome if we could have a test for the new setting too. You could have a look at |
|
Thanks for the attention and the feedback! I'm making the necessary changes. :) |
|
@areek could you review this again please? |
|
pinging @areek again for review :) |
| @@ -56,7 +59,14 @@ public TermSuggestion innerExecute(String name, TermSuggestionContext suggestion | |||
| ); | |||
| Text key = new BytesText(new BytesArray(token.term.bytes())); | |||
| TermSuggestion.Entry resultEntry = new TermSuggestion.Entry(key, token.startOffset, token.endOffset - token.startOffset); | |||
| for (SuggestWord suggestWord : suggestedWords) { | |||
| if (suggestion.getDirectSpellCheckerSettings().exactMatch()){ | |||
| final TermsEnum termsEnum = MultiFields.getTerms(indexReader, token.term.field()).iterator(); | |||
|
@ricardocerq sorry for the delay, this LGTM, would you mind updating the PR with master? |
|
@ricardocerq are you able to update this by rebasing or merging master in so it can be merged? |
|
Sorry about the delay :(. I was unable to retest the changes. If more changes are necessary, please let me know. |
|
@areek @clintongormley Can this be merged? |
| @@ -0,0 +1,301 @@ | |||
| /* | |||
There was a problem hiding this comment.
this entire class seems unused?
| @@ -137,6 +139,14 @@ public void minDocFreq(float minDocFreq) { | |||
| this.minDocFreq = minDocFreq; | |||
| } | |||
|
|
|||
| public boolean exactMatch() { | |||
There was a problem hiding this comment.
can we have javadocs on this?
| @@ -0,0 +1,1329 @@ | |||
| /* | |||
There was a problem hiding this comment.
this is a full copy of our tests that has been moved back to core into org.elasticsearch.search.suggest.SuggestSearchIT as far as I can tell only testExactMatch should be added?
|
@ricardocerq are you still interested in completing this PR? |
|
Closing in favour of #22902 |
Solves #11579.
Added option to term suggest that returns the input term if it exists. Turned off by default so it doesn't break normal behaviour.
The example query given in #11579 would now be:
And the result:
The output now clearly indicates if the term is present. If it is, its score is always 1, representing an exact match, which differentiates this result from other suggestions.